Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Variable length arithmetic #25

Merged
merged 5 commits into from
Feb 12, 2024
Merged

Conversation

JasperTey
Copy link
Contributor

Changed

  • Arithmetic expressions Add, Subtract, Multiply, Divide and Modulo now accept variable number of additional arguments beyond the first two

This PR implements support for variable-length arithmetic arguments where applicable, as discussed in #23.

I didn't end up applying this update to the bitwise operations even though it was initially deemed mathematically possible, for two reasons:

  • I questioned whether it's actually practical.
  • It didn't look intuitive when I saw the Xor syntax for sqlite (({$value1} | {$value2}) - ({$value1} & {$value2})); and I didn't want to go there for this PR.

I'm open to alternate naming conventions and/or testing methodology.

@JasperTey JasperTey marked this pull request as ready for review January 27, 2024 19:42
@tpetry
Copy link
Owner

tpetry commented Jan 30, 2024

Thanks for your PR @JasperTey. I am currently focused on my talk on Laracon EU next week. I max make some changes to your PR and merge it when coming back.

@tpetry tpetry merged commit df0fe4f into tpetry:main Feb 12, 2024
151 of 152 checks passed
@tpetry
Copy link
Owner

tpetry commented Feb 12, 2024

Thanks for your contribution. Its now released as 1.1.0.

I've made some small improvements:

  • Moved the abstract expression to Operator\Arithmetic\ArithmeticExpression
  • Added support for Power so all arithmetic expression support varargs
  • Simplified the tests by using just one variadic sample (its testing less cases but more easy to understand)

@JasperTey
Copy link
Contributor Author

Thanks for your contribution. Its now released as 1.1.0.

I've made some small improvements:

  • Moved the abstract expression to Operator\Arithmetic\ArithmeticExpression
  • Added support for Power so all arithmetic expression support varargs
  • Simplified the tests by using just one variadic sample (its testing less cases but more easy to understand)

Fantastic, thanks for refining further. I saw you add support for Power - awesome! A quick note that the Power syntax in the README should be updated since it wasn't accounted for in my initial PR.

image

@tpetry
Copy link
Owner

tpetry commented Feb 12, 2024

Youre right. Completely forgot updating the Readme. Fixed.

@JasperTey JasperTey deleted the variable-length-arithmetic branch February 26, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants